-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add command jj annotate/blame #4176
Conversation
As far as the test cases I've thrown at it, it matches with git blame but if anyone finds a counterexample, I'm happy to take a look and see where my bugs are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a initial comment, it should also mention that this starts work on #2988.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a second review.
I think that it should figure out which side of the conflict comes from which parent of the commit. Any sides that don't come from a parent (e.g. after a rebase) can be attributed as being added to the commit itself. Infrastructure related to #4062 would be helpful here. Of course, none of this is necessary for a first version of annotate functionality. |
Thanks for working on this! I have not looked at the implementation yet, but I'll just quickly note that I think a TUI for the functionality would be really useful. I quite often end up trying to find where the previous version of a line came from, and quite often I end up repeating that process several times. GitHub has a "Blame prior to change abc123..." button for each line (or span of lines) for that. It's probably not a big deal to redo the calculation when the user asks for that option, but ideally we could reuse some of the information we already calculated. |
That's a nice idea. I'm hoping to merge this in and maybe in a future PR I can implement a TUI? Unless you think implementing a TUI would radically change things enough to not make it worth it to merge this in now? |
Sounds good. (And I didn't mean to ask you to create a TUI, but sounds great if you're interested in doing that.)
Nope. I just mentioned it in case it would help any future work in this area. |
I agree with this for conflicts, I initially tried this but gave up for the first iteration as currently the conflict writers (lib/src/conflict.rs) doesn't have the commit information. So I'd have to forward that through the callers. Definitely doable but a little out of scope for this PR I think |
8090b5f
to
f62815a
Compare
684840d
to
ae48961
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not done reviewing yet, but here are my comments so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG to me after this round, but you should wait for a final approval from either Martin or Yuya.
b446272
to
4c5831a
Compare
176b802
to
6eaff71
Compare
843e419
to
407da8d
Compare
What are the remaining blockers here? As I'd really like to finally have a |
This and some related changes? I can rewrite the implementation as a follow up if he's okay. |
I was working on a few different implementations, none of which I was
really happy with so I'm going to fix it up to a state that I liked over
the next two days and then we can go over final review
…On Fri, Oct 11, 2024, 03:34 Yuya Nishihara ***@***.***> wrote:
What are the remaining blockers here? As I'd really like to finally have a
annotate command, even if it is not perfect yet.
This and some related changes?
#4176 (comment)
<#4176 (comment)>
I can rewrite the implementation as a follow up if he's okay.
—
Reply to this email directly, view it on GitHub
<#4176 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABM6VFBXCXZ3IGSDUUUUUQ3Z24MKHAVCNFSM6AAAAABLUSBBUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBWGMYDEMBZGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I think the last big issue remaining is command name. |
Yes, I would go with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
e9bbedf
to
3fea76c
Compare
A new module is added to jj_lib which exposes a function get_annotation_for_file. This annotates the given file line by line with commit information according to the commit that made the most recent change to the line. Similarly, a new command is added to the CLI called `jj file annotate` which accepts a file path. It then prints out line by line the commit information for the line and the line itself. Specific commit information can be configured via the templates.annotate_commit_summary config variable
@yuja addressed the last round of quick fixes. Anything more before merging? |
No. Thanks for updating it quickly. |
This change adds a new annotate command (in git this is blame).
In order to annotate a file, call:
jj annotate <FILE_PATH>
.In response, jj will traverse the commit graph, trying to find the source of the change for each line.
The output is something similar to this:
I've also added a new module to jj_lib which exposes a function
get_annotation_for_file
whichreturns a line by line evaluation of the the given file in a more general interface.
A few caveats
Checklist
If applicable:
CHANGELOG.md